Skip to content

Add Model type to Cadl.Reflection - #1524

Closed
Pan Shao (pshao25) wants to merge 2 commits into
microsoft:mainfrom
pshao25:reflectionModel
Closed

Add Model type to Cadl.Reflection#1524
Pan Shao (pshao25) wants to merge 2 commits into
microsoft:mainfrom
pshao25:reflectionModel

Conversation

@pshao25

Copy link
Copy Markdown
Contributor

We have such decorator to be added:

extern dec exclude(target: Model);

However, Model is not in the Cadl.Reflection

@github-actions

Copy link
Copy Markdown
Contributor

Changes in this PR will be published to the following url to try(check status of Cadl Pull Request Try It pipeline for publish status):
Playground: https://cadlplayground.z22.web.core.windows.net/prs/1524/

Website: https://cadlwebsite.z1.web.core.windows.net/prs/1524/

@allenjzhang

Copy link
Copy Markdown
Contributor

Brian Terlson (@bterlson), Timothee Guerin (@timotheeguerin), Nick Guerrera (@nguerrera), I had an earlier proposal like this, but Tim disagreed and mentioned that object implies model. I don't know if this is a TS convention, but I feel having model is clearer.

BTW, if this change is approved, we need to do a pass on existing libraries to update the decorators.

@nguerrera

Copy link
Copy Markdown
Contributor

One possible solution would be

namepsace Cadl.Reflection {
   alias Model = object;
}

@nguerrera

Copy link
Copy Markdown
Contributor

Also, just a note that I don't think the PR as is will achieve the desired result. I think at a minimum, ReflectionNameToKind needs to be updated, and this would need tests. But don't work on this until we have a design approved.

@nguerrera

Copy link
Copy Markdown
Contributor

I recommend closing this and opening a design issue.

@nguerrera Nick Guerrera (nguerrera) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@pshao25

Copy link
Copy Markdown
Contributor Author

Thanks all. I create issue at #1536. And close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants